feat: make remote evaluation connection pool size configurable - #77
Open
cmyui wants to merge 1 commit into
Open
Conversation
The remote evaluation client hardcodes HTTPConnectionPool(max_size=1), so every fetch in a process serializes through a single HTTP connection, and concurrent callers block in pool.acquire() with no timeout. Under concurrent server-side usage during a period of elevated API latency, queued callers can be blocked far longer than fetch_timeout_millis (which only bounds the socket read), pinning worker threads. Adds RemoteEvaluationConfig.connection_pool_max_size (default 1, preserving existing behavior) and passes it through to the connection pool, aligning with the Node server SDK where the https.Agent does not limit concurrent sockets. Co-Authored-By: Claude <noreply@anthropic.com>
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The remote evaluation client hardcodes its connection pool to a single connection:
and acquires it with no timeout, so every
fetch_v2in a process serializes through one HTTP connection and concurrent callers block indefinitely inpool.acquire().fetch_timeout_millisbounds the socket read only — not the queue wait — so under concurrent server-side usage during a period of elevated evaluation-API latency, queued callers can be blocked far longer than any configured timeout, pinning worker threads/greenlets. We hit exactly this in production: request handlers stalled insideconnection_pool.acquire()while the fetch timeout never applied.Observed impact
To illustrate the magnitude: during a ~10-minute window of elevated evaluation-API latency, this serialization amplified the slowdown into tens of thousands of failed requests across several of our services — the worst-affected service logged ~13,000 request errors, and a downstream dependent that had correctly bounded its own call timeouts logged ~4,700 more as cascade. Two properties made it unusually hard to diagnose:
acquire()never reach the socket, so no fetch error or timeout is ever recorded — monitoring built on fetch outcomes shows a healthy SDK while worker threads/greenlets starve underneath it.Because the pool size is hardcoded, no combination of the SDK's existing config options can mitigate this today.
Change
This PR adds
connection_pool_max_sizetoRemoteEvaluationConfig(default1, preserving existing behavior exactly) and passes it through to the pool, so concurrent server-side callers can size the pool to their needs:This also aligns the Python SDK with the Node server SDK, whose
https.Agent({ keepAlive: true })does not limit concurrent sockets.The pool implementation already supports arbitrary sizes — this only exposes the existing parameter through config. Tests added for the config default, config pass-through, and the resulting pool size on the client.
A natural follow-up would be bounding the
acquire()wait itself (e.g. applyingfetch_timeout_millisto queue wait, or a separateacquire_timeout_millis) so callers fail fast instead of queueing unboundedly — happy to open that separately if there's interest.Checklist
Note
Low Risk
Small, backward-compatible config surface with default behavior unchanged; only affects fetch concurrency and connection reuse.
Overview
Adds
connection_pool_max_sizetoRemoteEvaluationConfig(default 1, unchanged from the previous hardcoded value) and wires it intoRemoteEvaluationClient’sHTTPConnectionPoolinstead of a fixedmax_size=1.Callers can raise the limit so multiple concurrent
fetch_v2calls can use separate keep-alive connections instead of blocking onpool.acquire()when the single connection is busy. Docs on the config field describe that extra concurrent fetches wait for a released connection.Unit tests cover the config default/storage and that the client pool’s
max_sizereflects the setting.Reviewed by Cursor Bugbot for commit 12eb4b8. Bugbot is set up for automated code reviews on this repo. Configure here.